Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for custom Paperclip mappings #1271

Merged
merged 1 commit into from Nov 26, 2013
Merged

Add support for custom Paperclip mappings #1271

merged 1 commit into from Nov 26, 2013

Conversation

Robovirtuoso
Copy link
Contributor

Paperclip supports custom interpolations in images' paths. In order to
ease transition into CarrierWave, support the same functionality with the
Paperclip::Compatibility module.

The public api is similar to Paperclip's

class CustomUploader < CarrierWave::Uploader::Base
  include CarrierWave::Compatibility::Paperclip

  interpolate :some_method do |custom, style|
    custom.model.some_method
  end
end

This is accomplished by appending the desired functionality into
CarrierWave::Compatibility::Paperclip#mappings

In addition, we removed some double stated dependencies (timecop & nokogiri)
and resolved a problem with bundling dependencies for mime-types.

@@ -37,6 +37,4 @@ Gem::Specification.new do |s|
s.add_development_dependency "fog", ">= 1.3.1"
s.add_development_dependency "mini_magick", ">= 3.6.0"
s.add_development_dependency "rmagick"
s.add_development_dependency "nokogiri", "~> 1.5.10" # 1.6 requires ruby > 1.8.7
s.add_development_dependency "timecop", "0.6.1" # 0.6.2 requires ruby > 1.8.7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines, as indicated in the comment, are required for 1.8.7 compatibility

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't modify the gemspec, these are required for Ruby 1.8.7.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beat me @taavo!!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah! Btw I just opened a question on stack overflow re mime-types killing bundler: http://stackoverflow.com/questions/20107243/travis-error-bundler-sometimes-cant-find-compatible-versions. I'm open minded to reverting to => 1.16, as in the above, just so we can use travis again.

@stephenprater
Copy link

Embrace the future people. 😉

@bensie
Copy link
Member

bensie commented Nov 20, 2013

@stephenprater If we hit a roadblock where we can't continue without dropping support for 1.8, we'll certainly embrace the future. But CarrierWave supports Rails 3, which supports 1.8.

@Robovirtuoso
Copy link
Contributor Author

Tests are failing because Rails master depends on arel 5.0 which has not been released yet.

@@ -26,7 +26,7 @@ Gem::Specification.new do |s|
s.add_dependency "activesupport", ">= 3.2.0"
s.add_dependency "activemodel", ">= 3.2.0"
s.add_dependency "json", ">= 1.7"
s.add_dependency "mime-types", ">= 1.25"
s.add_dependency "mime-types", ">= 1.16"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this change, in this PR or otherwise. Although our gemspec used to work as is, something changed and bundle install now goes into an infinite loop or gives the same error travis has been reporting. I'd assumed this was just travis, but it happens for me locally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but let's put this in a separate PR since it'll probably come up again in the future and could really use its own description.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I committed this line and the spec_helper line below separately, because I was really tired of watching travis explode. @Robovirtuoso, can you rebase off master? Also: Thank you for fixing our build for us.

@bensie
Copy link
Member

bensie commented Nov 20, 2013

@Robovirtuoso No problem, I'll fix the issues with Rails master -- that happens pretty much anytime a point release is near.

@stephenprater
Copy link

We got a little further into this and realized that while the test case worked that in real code you'd have a problem with multiple versions - introduced a test case to cover that aspect of it and changed the implementation a little bit.

Should be good now.

@stephenprater
Copy link

One other thing we noticed was that in Paperclip's custom interpolations the second method is 'style' - like :thumb or whatever - but here's it's actually the base file name. Do we want to mimic the Paperclip implementation here or leave it as is?

@Robovirtuoso
Copy link
Contributor Author

The latest commit also rebased this branch against master

@thiagofm
Copy link
Member

Looking good! I think it should mimic the paperclip implementation.

Please don't forget to squash the commits and add a proper author(with a linked github account). 👍

@Robovirtuoso
Copy link
Contributor Author

The block variables in the .interpolate block now mimic that of Paperclip's implementation, commits have been squashed and an author with a link to a github was added.

Paperclip supports custom interpolations in images' paths. In order to
ease transition into CarrierWave, support the same functionality with the
`Paperclip::Compatibility` module.

The public api is similar to Paperclip's

```ruby
class CustomUploader < CarrierWave::Uploader::Base
  include CarrierWave::Compatibility::Paperclip

  interpolate :some_method do |custom, style|
    custom.model.some_method
  end
end
```

This is accomplished by appending the desired functionality into
`CarrierWave::Compatibility::Paperclip#mappings`
@Robovirtuoso
Copy link
Contributor Author

Changing the block variables broke some of the pre-built interpolators that did not have test coverage. Test coverage was added and interpolators were fixed.

bensie added a commit that referenced this pull request Nov 26, 2013
…ip_interoplations

Add support for custom Paperclip mappings
@bensie bensie merged commit cdcab9b into carrierwaveuploader:master Nov 26, 2013
@@ -47,6 +55,86 @@ module Rails; end unless defined?(Rails)
@uploader.stub!(:paperclip_path).and_return("/foo/:id_partition/bar")
@uploader.store_path("monkey.png").should == "/foo/000/000/023/bar"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Ruby 1.8 doesn't have ordered hashes, this particular spec example occasionally fails on 1.8.7. That's because sometimes the mapping for :id is used instead of the mapping for id_partition, causing the #store_path to return "/foo/23_partition/bar". Whoops.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants